fix: httpjson callables to trace attempts (started, failed) by diegomarquezp · Pull Request #3300 · googleapis/sdk-platform-java

diegomarquezp · 2024-10-21T20:21:18Z

Approach

This PR uses the approach suggested by the same bug.

We should move creating the TracedUnaryCallable to the last step for HttpJson. This would require refactoring this file: https://github.com/googleapis/sdk-platform-java/blob/7902a41c87240d607179d07c28cce462ea135c5f/gax-java/gax-httpjson/src/main/java/com/google/api/gax/httpjson/HttpJsonCallableFactory.java

Confirmation that it works

The confirmation that it works is in the modified tests (e.g. retry()). When using the proposed test version against the version of HttpJsonCallableFactory from the main branch, it will fail because it will not record attempts started or failed whatsoever:

The image above shows all tests having failed when using the production files from main, implying that no tracer attempts are recorded for any of the tests.

…trics

lqiu96 · 2024-10-22T18:06:47Z

I think you can also remove a bunch of the @Disabled annotations in the showcase ITs as well.

sdk-platform-java/showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITOtelMetrics.java

Line 323 in 7512cfa

@Disabled("https://github.com/googleapis/sdk-platform-java/issues/2503")

I think they should pass with your PR.

…eapis/sdk-platform-java into fix-httpjson-retry-metrics

lqiu96

~~Can you try and re-enable the Disabled ITOtelMetric tests and see if it works? LGTM otherwise~~

Edit: Spoke too soon.

…metrics' into fix-httpjson-retry-metrics

…eapis/sdk-platform-java into fix-httpjson-retry-metrics

…trics

lqiu96 · 2024-10-28T14:06:32Z

+    // the number of attempts varies. Here we just make sure that all of them except the last are
+    // considered as failed


qq, isn't the last one also considered a failed attempt? Why is the tracerAttemptsFailed != tracerAttempts?

Also, can we add the operationfailed == true check here as well?

Why is the tracerAttemptsFailed != tracerAttempts?

I'm not sure and I agree they should be the same value since this test just fetches an exception until it can't retry anymore (retries exhausted).

from discussion with @blakeli0, @lqiu96 and @zhumin8, let's double check TestApiTracer, confirm with showcase.

showcase has another tracer we can use to cross check TestApiTracer

lqiu96

LGTM. Added a few questions/ nits

blakeli0 · 2024-10-30T22:02:37Z

+ */
+public class TestApiTracer extends BaseApiTracer {
+
+  private final AtomicInteger tracerAttempts;


I think they can be called just attempts, same for the fields below.

renamed to attemptsStarted, removed the tracer prefix from all fields

blakeli0 · 2024-10-30T22:05:14Z

+          tracerOperationFailed,
+          tracerFailedRetriesExhausted);
+
+  public AtomicInteger getTracerAttempts() {


I think it's better to get the tracer instance, and use the getters from the tracer in unit tests.

blakeli0 · 2024-10-30T22:05:47Z

+ * exhausted.
+ */
+public class TestApiTracerFactory implements ApiTracerFactory {
+  private final AtomicInteger tracerAttempts = new AtomicInteger();


These can be initialized within the tracer for better encapsulation.

I'd like to initialize them in the tracer itself. However the clients only expect tracer factories, so there is no way to have control over how the actual tracers are instantiated.
I'll double check on this.

sdk-platform-java/gax-java/gax-httpjson/src/test/java/com/google/api/gax/httpjson/RetryingTest.java

Lines 121 to 128 in 2df2e9f

clientContext =

ClientContext.newBuilder()

// we use a custom tracer to confirm whether the retrials are being recorded.

.setTracerFactory(tracerFactory)

.setExecutor(executor)

.setClock(fakeClock)

.setDefaultCallContext(HttpJsonCallContext.createDefault())

.build();

Ah nvm, I realized that accessing the ApiTracer instance should be enough.

blakeli0 · 2024-10-30T22:09:23Z

+  }
+
+  @Override
+  public void attemptStarted(int attemptNumber) {


Use attemptStarted(Object request, int attemptNumber) instead

blakeli0 · 2024-10-30T22:15:52Z

I like the idea of a test tracer in general. On the other hand, we should only implement what is needed for our use cases, so that we have lower chance to run into issues that are not related to our PR. For example, maybe we don't need to implement attemptFailedRetriesExhausted.

@lqiu96 figured that the retries exhausted event records the missing failed attempt in our tests:

It looks like attempt_count is recorded when this is called:

sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/tracing/MetricsTracer.java

Line 204 in 4d4c798

metricsRecorder.recordAttemptCount(1, attributes);

. This seems like the number recorded in Otel should be correct.
I'm guessing your TestApiTracer would need to increment the attemptCount when attemptFailedRetriesExhausted is called as well to match behavior.

I'll keep this implementation and its retries exhausted flag.

blakeli0 · 2024-10-30T22:16:13Z

+ * Test tracer that keeps count of different events. See {@link TestApiTracerFactory} for more
+ * details.
+ */
+public class TestApiTracer extends BaseApiTracer {


We should use the interface ApiTracer instead.

This reverts commit e57b4db.

lqiu96 · 2024-11-06T22:15:09Z

FYI @diegomarquezp
CI error:

Error:  Failures: 
Error:    RetryingTest.retryKeepFailing:360 expected to be true

sonarqubecloud · 2024-11-11T22:59:23Z

Quality Gate passed for 'gapic-generator-java-root'

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarqubecloud · 2024-11-11T23:03:00Z

Quality Gate passed for 'java_showcase_integration_tests'

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

blakeli0

LGTM. Please make sure @lqiu96 has no other concerns before merging.

diegomarquezp · 2024-11-13T23:28:43Z

From @lqiu96 (thanks!):

I have a suspicion as to why the race condition exists. I took a look into the code and I think I understand what you were explaining in the call.
My suspicion of the cause of the race condition:

sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/retrying/BasicRetryingFuture.java

Line 205 in bb2a3be

super.set(response);

This sets the result to the future which frees up the thread that is running the test. (basically .get() no longer blocks and if the thread schedules the test to run, it will). The tracerFinisher code which sets the operation failure status runs after this

This means that the tests removed in 065b01f were actually expected behavior and is due to the way we set up the tests.

diegomarquezp added 3 commits October 21, 2024 16:07

add test to confirm tracer attempts failed

6635019

simplify test for retries

944e8d6

Merge remote-tracking branch 'origin/main' into fix-httpjson-retry-me…

1edf557

…trics

product-auto-label Bot added the size: s Pull request size is small. label Oct 21, 2024

extend tracer tests to every test

4db1d2b

product-auto-label Bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Oct 21, 2024

diegomarquezp added 2 commits October 21, 2024 16:59

fix test vars initialization

9ab23d5

add retrial exhaustion check

6364aab

lqiu96 reviewed Oct 22, 2024

View reviewed changes

Comment thread gax-java/gax-httpjson/src/test/java/com/google/api/gax/httpjson/RetryingTest.java Outdated

diegomarquezp added 3 commits October 22, 2024 19:48

Merge branch 'main' into fix-httpjson-retry-metrics

ae1eb44

restore ApiException expected in test

1ebc3a2

Merge branch 'fix-httpjson-retry-metrics' of https://github.com/googl…

25e778f

…eapis/sdk-platform-java into fix-httpjson-retry-metrics

lqiu96 reviewed Oct 24, 2024

View reviewed changes

Comment thread gax-java/gax-httpjson/src/test/java/com/google/api/gax/httpjson/RetryingTest.java Outdated

enable Disabled ITs

b363a29

lqiu96 reviewed Oct 24, 2024

View reviewed changes

try fixing multipleattempts test

5c11e10

diegomarquezp commented Oct 24, 2024

View reviewed changes

Comment thread showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITOtelMetrics.java

diegomarquezp closed this Oct 24, 2024

diegomarquezp reopened this Oct 24, 2024

diegomarquezp added 3 commits October 24, 2024 18:10

Merge branch 'main' into fix-httpjson-retry-metrics

9b48dbe

fix multiple attempts httpjson test (thanks @lqiu96)

c8e8a7d

Merge remote-tracking branch 'refs/remotes/origin/fix-httpjson-retry-…

af3adb5

…metrics' into fix-httpjson-retry-metrics

diegomarquezp marked this pull request as ready for review October 25, 2024 23:32

diegomarquezp added 5 commits October 25, 2024 19:50

extract tracer factory to its own method

035be17

add comments

8e2a58a

Merge branch 'fix-httpjson-retry-metrics' of https://github.com/googl…

8f3331e

…eapis/sdk-platform-java into fix-httpjson-retry-metrics

Merge remote-tracking branch 'origin/main' into fix-httpjson-retry-me…

f647320

…trics

licence

2df2e9f

lqiu96 reviewed Oct 28, 2024

View reviewed changes

formalize tests

e7bc1c3

blakeli0 reviewed Oct 30, 2024

View reviewed changes

diegomarquezp added 7 commits October 31, 2024 17:25

rename vars, extend ApiTracer

e2385ad

move initialization to contained class

58e874a

fix order of methods

87f807a

remove retries exhausted

e57b4db

Revert "remove retries exhausted"

58a0daa

This reverts commit e57b4db.

record attempt failed when retries are exhausted

325eba4

adjust retries count in retryingtest

2120960

diegomarquezp requested review from blakeli0 and lqiu96 November 5, 2024 23:19

diegomarquezp added 3 commits November 11, 2024 10:53

Merge branch 'main' into fix-httpjson-retry-metrics

0630fdf

empty commit to retry tests

9697e8a

remove flaky tests

065b01f

blakeli0 approved these changes Nov 12, 2024

View reviewed changes

lqiu96 approved these changes Nov 13, 2024

View reviewed changes

diegomarquezp merged commit 15a64ee into main Nov 13, 2024

diegomarquezp deleted the fix-httpjson-retry-metrics branch November 13, 2024 23:29

release-please Bot mentioned this pull request Nov 13, 2024

chore(main): release 2.50.0 #3327

Merged

suztomo mentioned this pull request Nov 5, 2025

chore(2.59.x): release 2.59.3 #3982

Merged

release-please Bot mentioned this pull request Dec 11, 2025

chore(protobuf-4.x-rc): release 2.65.0-rc1 #3999

Merged

		// the number of attempts varies. Here we just make sure that all of them except the last are
		// considered as failed

	clientContext =
	ClientContext.newBuilder()
	// we use a custom tracer to confirm whether the retrials are being recorded.
	.setTracerFactory(tracerFactory)
	.setExecutor(executor)
	.setClock(fakeClock)
	.setDefaultCallContext(HttpJsonCallContext.createDefault())
	.build();

Conversation

diegomarquezp commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Approach

Confirmation that it works

Uh oh!

Uh oh!

lqiu96 commented Oct 22, 2024

Uh oh!

Uh oh!

lqiu96 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqiu96 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lqiu96 commented Nov 6, 2024

Uh oh!

sonarqubecloud Bot commented Nov 11, 2024

Quality Gate passed for 'gapic-generator-java-root'

Uh oh!

sonarqubecloud Bot commented Nov 11, 2024

Quality Gate passed for 'java_showcase_integration_tests'

Uh oh!

blakeli0 left a comment

Choose a reason for hiding this comment

Uh oh!

diegomarquezp commented Nov 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

diegomarquezp commented Oct 21, 2024 •

edited

Loading

lqiu96 left a comment •

edited

Loading